Skip to content

Conversation

yardenshoham
Copy link
Member

  • Following Run make fmt #21437 I realized the build system isn't actually checking that the code is formatted

@yardenshoham yardenshoham marked this pull request as ready for review October 14, 2022 10:59
@silverwind
Copy link
Member

silverwind commented Oct 14, 2022

Instead of adding another drone step, just add it to checks-backend as a dependency:

gitea/Makefile

Line 337 in 9dc264a

checks-backend: tidy-check swagger-check swagger-validate

E.g.

 checks-backend: tidy-check swagger-check fmt-check swagger-validate

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 14, 2022
@yardenshoham yardenshoham changed the title Add fmt-check to Drone pipeline Add fmt-check to make checks-backend for CI Oct 14, 2022
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 14, 2022
@silverwind silverwind added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Oct 14, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 14, 2022
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we also need to add fmt to the dependencies of fmt-check, otherwise it will never see a diff.

@yardenshoham
Copy link
Member Author

Hmmm, I see, perhaps having a different way to handle templates formatting is not the best way to go. Simply having fmt being called during fmt-check drastically changes its behavior. Wrong Go formatting won't be detected in Drone at all. The best thing to do would be to have gitea-fmt include template formatting.

@silverwind
Copy link
Member

I'm not sure what you're getting at, but we use the same strategy already for other things like tidy-check which has the tidy dependency. When running tidy-check, make will just trigger the tidy target before it, nur "during" it.

So I think having fmt-check: fmt on line 315 is totally fine.

@yardenshoham
Copy link
Member Author

Suppose I commit a changeset with wrongly formatted Go code and correctly formatted templates, fmt runs, formatting my Go code on the CI filesystem and then fmt-check runs (reporting no errors as fmt formatted my bad Go code and templates is formatted correctly) and exits with success.

@silverwind
Copy link
Member

silverwind commented Oct 14, 2022

Ah, I see fmt-check also runs the go formatting again, I missed that. So the solution should then be remove the go formatting in fmt-check, so that only the diffing is left, then add the dependency which in turn will make it work just like tidy-check does while keeping DRY as well :)

@silverwind
Copy link
Member

silverwind commented Oct 14, 2022

I suppose you can just reduce to a single git diff call as well:

@diff=$$(git diff $(GO_SOURCES) templates); \
if [ -n "$$diff" ]; then \
  echo "Please run 'make fmt' and commit the result:"; \
  echo "$${diff}"; \
  exit 1; \
fi

@yardenshoham
Copy link
Member Author

How about we merge this as-is and open an issue on how to handle the actual fmt-check logic? This PR is only about adding fmt-check to CI

Signed-off-by: Yarden Shoham <[email protected]>
@silverwind
Copy link
Member

#21458 for the proposed enhancements. Fine with me to land this before.

@silverwind
Copy link
Member

I suggest we just land #21458 instead of this. The order of targets is different here, so if this lands my PR would conflict.

@silverwind silverwind closed this Oct 15, 2022
@yardenshoham yardenshoham deleted the drone-fmt-check branch October 15, 2022 09:26
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants